⚡ http client defer CloseIdleConnections #1513
Conversation
There was a problem hiding this comment.
Hm, my understanding is that this change would only makes things slower, will not help with too many concurrent connections (if you mean that).
Perhaps a reproduction test would help? Perhaps there is some http transport settings you can adjust, or you hit some other limits?
api/client.go
Outdated
| @@ -122,6 +122,7 @@ func (c *httpClient) Do(ctx context.Context, req *http.Request) (*http.Response, | |||
| if ctx != nil { | |||
| req = req.WithContext(ctx) | |||
| } | |||
| defer c.client.CloseIdleConnections() | |||
There was a problem hiding this comment.
Are you sure this is a correct solution?
My understanding is that this closes connections which might be REUSED on the subsequent Do request. So you have to establish them again (overhead)?
Source: https://forum.golangbridge.org/t/when-should-i-use-client-closeidleconnections/19254, https://stackoverflow.com/questions/18697290/apache-httpclient-how-to-auto-close-connections-by-servers-keep-alive-time
There was a problem hiding this comment.
Currently, doing an HTTP probe requiring allocating a new http.Client and http.Transport, and the underlying connections managed by http.Transport might be ESTABLISHED indefinitely if no parties (client or server) directly close them. As a result, if the probes continuously happen, we can see TCP connections opened by one process
drastically increase.
I understand that there may be such a problem in high concurrency. Even if it is reused, the connection is limited and cannot meet the requirements. In this case, the connection may be occupied and new connections cannot be applied for. As a result, the connection request cannot be continued.
PS. I appeared in a high-concurrency scenario, with about 10000 of concurrencies.
There was a problem hiding this comment.
Nice! What "HTTP probe" do you mean? Is it some OSS code or your own app?
Generally it sounds you might use this client incorrectly if you create new client for single Do, assuming you can share the client.
For cases you cannot share e.g. it's serverless or it's on-off request to adhoc destination, I think you can access Config.Client (which is used to create API client) and invoke this method after Do on your own, which would be a correct thing to do 👍🏽
I am also open to create CloseIdleConnections() method on our client with a clear commentary when it's useful to make it easier (:
Would that help?
There was a problem hiding this comment.
in this conde add Close func ?
// Client is the interface for an API client.
type Client interface {
URL(ep string, args map[string]string) *url.URL
Do(context.Context, *http.Request) (*http.Response, []byte, error)
}
I think it would be more reasonable if the interface or open functions could be adjusted and allowed to be called by users themselves. Very useful to me too.
There was a problem hiding this comment.
@bwplotka i am add code to interface using CloseIdleConnections() func . plz review this code ?
There was a problem hiding this comment.
Why do we need interface change? Can we remove this defer? Otherwise new method is good 👍🏽
There was a problem hiding this comment.
Ok interface change is needed. Let's kill the defer statement here and LGTM!
cuisongliu
force-pushed
the
close_client
branch
3 times, most recently
from
274e000 to
05e818a
Compare
api/client.go
Outdated
| @@ -75,6 +75,7 @@ func (cfg *Config) validate() error { | |||
|
|
|||
| // Client is the interface for an API client. | |||
| type Client interface { | |||
| CloseIdleConnections() | |||
There was a problem hiding this comment.
Let's move it down (as 3rd in order)
api/client.go
Outdated
| @@ -122,6 +122,7 @@ func (c *httpClient) Do(ctx context.Context, req *http.Request) (*http.Response, | |||
| if ctx != nil { | |||
| req = req.WithContext(ctx) | |||
| } | |||
| defer c.client.CloseIdleConnections() | |||
There was a problem hiding this comment.
Ok interface change is needed. Let's kill the defer statement here and LGTM!
api/prometheus/v1/api.go
Outdated
| @@ -1342,6 +1342,7 @@ type Warnings []string | |||
| // apiClient wraps a regular client and processes successful API responses. | |||
| // Successful also includes responses that errored at the API level. | |||
| type apiClient interface { | |||
There was a problem hiding this comment.
Do we need this change on both interfaces really? Ideally those are very small.
cuisongliu
force-pushed
the
close_client
branch
2 times, most recently
from
ac1acdf to
4029570
Compare
api/client.go
Outdated
| @@ -79,6 +79,10 @@ type Client interface { | |||
| Do(context.Context, *http.Request) (*http.Response, []byte, error) | |||
| } | |||
|
|
|||
| type closeIdler interface { | |||
There was a problem hiding this comment.
| type closeIdler interface { | |
| type CloseIdler interface { |
There was a problem hiding this comment.
This allows us to avoid shallow ClientCloseIdler function.
api/client.go
Outdated
| func ClientCloseIdler(cl Client) { | ||
| cl.(closeIdler).CloseIdleConnections() | ||
| } | ||
|
|
There was a problem hiding this comment.
| func ClientCloseIdler(cl Client) { | |
| cl.(closeIdler).CloseIdleConnections() | |
| } | |
| func ClientCloseIdler(cl Client) { | |
| cl.(closeIdler).CloseIdleConnections() | |
| } |
No need for that, anyone can do cl.(CloseIdler).CloseIdleConnection()
…tion is rejected. Signed-off-by: cuisongliu <[email protected]>
| @@ -79,6 +79,10 @@ type Client interface { | |||
| Do(context.Context, *http.Request) (*http.Response, []byte, error) | |||
| } | |||
|
|
|||
| type CloseIdler interface { | |||
There was a problem hiding this comment.
Ideally we can comment what this is for, but we can do in a separare PR, thanks!
…ses. (prometheus#1513) Signed-off-by: cuisongliu <[email protected]> Signed-off-by: Eugene <[email protected]>
when high concurrency occurs, the connection is full and the connection is rejected.